Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure changes to the status attribute are written to the transport. #656

Closed
wants to merge 1 commit into from

Conversation

nhoad
Copy link

@nhoad nhoad commented Dec 4, 2015

I'm writing a proxy with aiohttp, and I need the possibility of modifying the response status code under certain conditions, something that currently isn't possible.

Personally I'm not a big fan for how I've done the handling of the reason of the attribute, but I can't think of a better way to do this.

@asvetlov
Copy link
Member

Please elaborate why do you need to modify status code?
What's your use case?

From my perspective http proxy can write different status line if needed without changing aiohttp.
Adding status_line property doesn't make any harm but you can keep formatting code in your application. I feel the need for status_line is very rare.

@nhoad
Copy link
Author

nhoad commented Dec 12, 2015

By default the proxy sets the status code to 200, assuming success. It sends a modified version of the request upstream, which may or may not return a 200, at which point the status code to be sent to the client needs to be modified.

Without this patch, the only way to do that is by modifying status_line directly, which is in my opinion much more dangerous.

@asvetlov
Copy link
Member

I still believe proxy should get response from upstream and send a copy of it to client.

No need for inplace request/response modification. All work can be done without aiohttp changing.

Even if status_line updating may be convenient for your particular code the use case is not too common for adding such hackery patch.

@nhoad
Copy link
Author

nhoad commented Dec 17, 2015

Then as an alternative, modifying the status attribute should be forbidden. It should be a readonly property, to make it clear that you're not supposed to modify it.

@asvetlov
Copy link
Member

Converting status to readonly makes sense.

@asvetlov asvetlov mentioned this pull request Dec 26, 2015
@asvetlov
Copy link
Member

Fixed by #710

@asvetlov asvetlov closed this Dec 27, 2015
@lock
Copy link

lock bot commented Oct 29, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants